Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a 'fly mode' feature to vanish. The implementation is mostly correct, but there is a significant vulnerability in the FlyController that allows players to gain flight in survival mode. I've provided a detailed explanation and a suggested fix for this issue across two comments. The changes to the config and interface are fine.
| @EventHandler(ignoreCancelled = true) | ||
| void onDisable(DisableVanishEvent event) { | ||
| Player player = event.getPlayer(); | ||
|
|
||
| if (player.getGameMode() == GameMode.CREATIVE || player.getGameMode() == GameMode.SPECTATOR) { | ||
| this.previousFlightStates.remove(player.getUniqueId()); | ||
| return; | ||
| } | ||
|
|
||
| FlightState state = this.previousFlightStates.remove(player.getUniqueId()); | ||
| if (state == null) { | ||
| return; | ||
| } | ||
|
|
||
| player.setAllowFlight(state.hadAllowFlight()); | ||
| player.setFlying(state.wasFlying()); | ||
| } |
There was a problem hiding this comment.
The current logic allows for a flight exploit where a player can enable vanish in creative mode, switch to survival, and retain flight after disabling vanish. By using the GameMode stored when vanish was enabled (as per my other comment), you can prevent this. The new logic should check if the player has switched from a gamemode that allows flight to one that doesn't, and if so, explicitly disable flight.
@EventHandler(ignoreCancelled = true)
void onDisable(DisableVanishEvent event) {
Player player = event.getPlayer();
FlightState state = this.previousFlightStates.remove(player.getUniqueId());
if (state == null) {
return;
}
GameMode currentMode = player.getGameMode();
if (currentMode == GameMode.CREATIVE || currentMode == GameMode.SPECTATOR) {
return;
}
GameMode originalMode = state.gameMode();
if (originalMode == GameMode.CREATIVE || originalMode == GameMode.SPECTATOR) {
player.setAllowFlight(false);
player.setFlying(false);
return;
}
player.setAllowFlight(state.hadAllowFlight());
player.setFlying(state.wasFlying());
}| this.previousFlightStates.computeIfAbsent( | ||
| player.getUniqueId(), | ||
| id -> new FlightState(player.getAllowFlight(), player.isFlying()) | ||
| ); |
There was a problem hiding this comment.
To fix a potential flight exploit, it's necessary to also store the player's GameMode when they enter vanish. This will be used when they exit vanish to determine if their flight status should be restored.
You will also need to update the FlightState record on line 68 to include a GameMode field:
private record FlightState(boolean hadAllowFlight, boolean wasFlying, GameMode gameMode) {}| this.previousFlightStates.computeIfAbsent( | |
| player.getUniqueId(), | |
| id -> new FlightState(player.getAllowFlight(), player.isFlying()) | |
| ); | |
| this.previousFlightStates.computeIfAbsent( | |
| player.getUniqueId(), | |
| id -> new FlightState(player.getAllowFlight(), player.isFlying(), player.getGameMode()) | |
| ); |
| class FlyController implements Listener { | ||
|
|
||
| private final VanishSettings settings; | ||
| private final Map<UUID, FlightState> previousFlightStates = new ConcurrentHashMap<>(); |
3 participants
No description provided.